Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: establish RTT at start of connection #64

Merged
merged 2 commits into from
Nov 14, 2023

Conversation

achingbrain
Copy link
Collaborator

A connection ping is required to establish the RTT - as implemented the ping only happens during the keep alive loop that doesn't kick in until after 30s by default.

The change is to ping immediately on sinking a source - this means we are more likely to increase the send window on the remote which increases the amount of data they send and as such, throughput.

A connection ping is required to establish the RTT - as implemented
the ping only happens during the keep alive loop that doesn't kick
in until after 30s by default.

The change is to ping immediately on sinking a source - this means
we are more likely to increase the send window on the remote which
increases the amount of data they send and as such, throughput.
@achingbrain achingbrain requested a review from a team as a code owner November 13, 2023 18:54
@@ -274,7 +274,7 @@ export class YamuxStream extends AbstractStream {
// then we (up to) double the recvWindow
const now = Date.now()
const rtt = this.getRTT()
if (flags === 0 && rtt > 0 && now - this.epochStart < rtt * 4) {
if (flags === 0 && rtt > -1 && now - this.epochStart < rtt * 4) {
Copy link
Collaborator Author

@achingbrain achingbrain Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RTT might be less than a ms on a very fast local connection?

🤷

@@ -45,9 +45,9 @@ describe('stream', () => {
// the window capacities should have refilled via window updates as received data was consumed

// eslint-disable-next-line @typescript-eslint/dot-notation
expect(c1['sendWindowCapacity']).to.equal(defaultConfig.initialStreamWindowSize)
expect(c1['sendWindowCapacity']).to.be.gte(defaultConfig.initialStreamWindowSize)
Copy link
Collaborator Author

@achingbrain achingbrain Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests have become non-deterministic after this change, sometimes the window is the starting 256KB, sometimes it's been increased to 16MB by the time the test finishes.

@achingbrain
Copy link
Collaborator Author

achingbrain commented Nov 13, 2023

I think this is one of the causes of the speed discrepancy between the js yamux and mplex implementations.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
src/muxer.ts 90.46% <100.00%> (+1.39%) ⬆️
src/stream.ts 98.01% <100.00%> (ø)
test/stream.spec.ts 100.00% <100.00%> (ø)

📢 Thoughts on this report? Let us know!

@wemeetagain wemeetagain merged commit 672523b into master Nov 14, 2023
17 checks passed
@wemeetagain wemeetagain deleted the fix/establish-rtt-at-start-of-connection branch November 14, 2023 13:29
github-actions bot pushed a commit that referenced this pull request Nov 14, 2023
## [5.0.3](v5.0.2...v5.0.3) (2023-11-14)

### Bug Fixes

* establish RTT at start of connection ([#64](#64)) ([672523b](672523b))
Copy link

🎉 This PR is included in version 5.0.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants